feat(web): add a reset-all button to keybindings settings - #4616
feat(web): add a reset-all button to keybindings settings#4616m-de-graaff wants to merge 4 commits into
Conversation
There is no way to get back to the stock keybindings from the GUI. A user carrying customizations from an older version can end up with conflicting bindings and no recourse short of hand-editing keybindings.json — which the settings screen already links to, but which is exactly what someone hitting this problem does not want to do. Adds `server.resetKeybindings`, which rewrites the config with the default rule set, and a reset button beside Add keybinding and Open keybindings.json. The action confirms first, since it discards every customization at once. Project script bindings (`script.*`) are kept. They have no default to restore, so dropping them would delete shortcuts the reset cannot give back. A config that fails to parse is replaced outright rather than erroring — that is the state the reset exists to escape. Closes pingdotgg#4576 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review This PR introduces a new user-facing feature (reset-all keybindings button) with a new RPC endpoint, backend logic, and UI components. New features introducing user-facing behavior warrant human review even when well-tested and self-contained. You can customize Macroscope's approvability policy. Learn more. |
…ax cap Three review findings, all real: - The reset swallowed every load failure, not just parse failures. A transient read error would resolve to "no rules" and the write would then drop the user's script bindings. It now loads through the runtime loader, which returns an empty rule set for a config it cannot parse but still fails on a filesystem error, so an unreadable config aborts the reset instead of overwriting it. - Capping with slice(-MAX) kept the trailing script rules and dropped default rules from the front, so a reset could fail to restore the defaults it exists to restore. Truncate the preserved scripts instead, and log what was dropped. - The web handler captured the primary environment id before awaiting the confirm dialog. Switching environments while the dialog was open would reset the wrong one. It now re-checks the current environment after the dialog resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three findings were correct. Fixed in 90f7248.
The reset now goes through The max-count cap dropped defaults, not scripts — Stale environment id across the confirm dialog — the handler captured Two tests added:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 90f7248. Configure here.
| // Truncate the preserved scripts, never the defaults — a reset that | ||
| // dropped default rules to stay under the cap would defeat itself. | ||
| const scriptBudget = Math.max(0, MAX_KEYBINDINGS_COUNT - DEFAULT_KEYBINDINGS.length); | ||
| const cappedConfig = [...DEFAULT_KEYBINDINGS, ...preservedScripts.slice(0, scriptBudget)]; |
There was a problem hiding this comment.
Script truncation drops highest-precedence rules
Low Severity
The resetKeybindingRulesToDefaults function truncates script.* keybindings by keeping the earliest entries (slice(0, budget)). This conflicts with the system's keybinding resolution, which prioritizes later rules, potentially discarding effective script shortcuts for older, overridden ones.
Reviewed by Cursor Bugbot for commit 90f7248. Configure here.
| if (!confirmed) return; | ||
| // The primary environment can change while the dialog is open, and this | ||
| // discards every customization — target only what the user was shown. | ||
| if (primaryEnvironmentRef.current?.environmentId !== environmentId) return; |
There was a problem hiding this comment.
Silent reset abort after confirm
Low Severity
After the user confirms reset, if primaryEnvironment changes during the async confirm dialog, the handler returns without calling the RPC and without a toast. environmentId from click time is already captured, so the confirmed reset is abandoned with no feedback.
Reviewed by Cursor Bugbot for commit 90f7248. Configure here.
…borted reset Two follow-up review findings: - Script truncation kept the earliest rules, but later rules have higher precedence throughout this file, so the cap could discard the script bindings actually in effect and keep the ones they shadow. Drop from the front instead, and pin the direction in the test. - The environment guard added after the confirm dialog returned silently. A user who confirmed a reset that then did not run had no way to tell. Toast instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both follow-up findings were correct. Fixed in 327f00c. Script truncation dropped the wrong end. I capped with The existing cap test passed either way, which is why it did not catch this. It now asserts the surviving scripts are exactly the trailing ones, so the direction is pinned. Silent abort after confirm. The environment guard I added in the previous round returned without feedback, so a user who confirmed a reset that then did not run had no way to tell. It now toasts "Keybindings were not reset — the active environment changed while the confirmation was open."
|


Closes #4576
Problem
Keybindings settings can reset a single row to its default, but there is no way to get the whole set back. A user carrying customizations from an older version can end up with conflicting bindings and no recourse short of hand-editing
keybindings.json— which is exactly what someone in that state does not want to do, and which the issue calls out directly.Change
server.resetKeybindingsRPC →Keybindings.resetKeybindingRulesToDefaults, which rewrites the config withDEFAULT_KEYBINDINGS, refreshes the cache, and emits the change event like the existing upsert/remove paths.Two deliberate behaviors, both covered by tests:
script.*) are kept. They have no default to restore, so dropping them would delete shortcuts the reset cannot give back. The confirm dialog says so.keybindings.jsonis the state this button exists to escape, so refusing to overwrite it would defeat the feature.upsertKeybindingRulekeeps its existing refuse-to-overwrite behavior.Tests
apps/server/src/keybindings.test.ts:script.*rule;vp test run src/keybindings.test.ts— 23 passed. (fails when config directory is not writablefails on this Windows host both with and without this change:fs.chmoddoes not restrict the directory there. Verified against a clean tree.)Typecheck clean for
packages/contracts,packages/client-runtime,apps/server,apps/web; lint and format checked on the changed files.Verified in the app
Ran the web stack against an isolated
--home-dir, planted a customizedkeybindings.json(remappedsidebar.toggleandterminal.toggle, plus ascript.run-tests.runbinding), and clicked the button:DEFAULT_KEYBINDINGS, the remapped shortcuts are gone, andscript.run-tests.runsurvives;Not included
The issue also floats migrations for defaults that change upstream. That is a separate mechanism and is not part of this change; the startup sync still backfills newly added defaults as it did before.
🤖 Generated with Claude Code
Note
Medium Risk
Bulk-writes user keybinding config and overwrites malformed files on reset; behavior is intentional but can surprise users who relied on a broken on-disk file.
Overview
Adds reset all keybindings to defaults end-to-end: a new
server.resetKeybindingsRPC (orchestration operate scope) wired through contracts, client-runtime, and the Keybindings settings header.Server-side
resetKeybindingRulesToDefaultsrewriteskeybindings.jsonwithDEFAULT_KEYBINDINGS, keepsscript.*rules, updates cache, and broadcasts changes like upsert/remove. Malformed configs are replaced on reset (unlike upsert, which still refuses to overwrite). Unreadable configs fail without writing. If script rules exceed the cap after reset, defaults are never dropped—oldest script entries are trimmed and a warning is logged.The settings UI adds a Reset all to defaults control with confirmation (notes that script bindings stay), guards against environment changes while the dialog is open, and surfaces errors via toasts.
Tests cover happy path, malformed replacement, read failure, and max-count script truncation.
Reviewed by Cursor Bugbot for commit 1e832d8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add a reset-all button to keybindings settings
resetKeybindingRulesToDefaultsin keybindings.ts: writes defaults plus any existing script bindings, updates the in-memory cache, and broadcasts a change event.server.resetKeybindingsWebSocket RPC in ws.ts, gated by orchestration operate scope.MAX_KEYBINDINGS_COUNT; excess script rules are dropped (trailing rules survive) with a warning logged.Macroscope summarized 1e832d8.